Skip to content

Gracefully handle common errors in credential values #1026

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Jan 9, 2019

Conversation

lpatino10
Copy link
Contributor

The functional change in this PR is that exceptions are thrown for credential values surrounded by {} or "" characters. This prevents making a call to the service and provides a more helpful error message.

Besides that, there was a lot of refactoring done with the credential logic in the SDK. Things had gotten a bit messy with the various changes we've had in auth methods and one-off fixes, so I consolidated some things to make it a bit clearer to work with. This should also make it easier to add the next auth feature coming soon 👀

mediumTaj
mediumTaj previously approved these changes Jan 8, 2019
Copy link
Contributor

@mediumTaj mediumTaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 looks good!

@codecov-io
Copy link

codecov-io commented Jan 9, 2019

Codecov Report

Merging #1026 into master will increase coverage by 0.03%.
The diff coverage is 62.5%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1026      +/-   ##
============================================
+ Coverage     66.14%   66.18%   +0.03%     
+ Complexity     3468     3466       -2     
============================================
  Files           739      739              
  Lines         17443    17425      -18     
  Branches        963      961       -2     
============================================
- Hits          11537    11532       -5     
+ Misses         5366     5357       -9     
+ Partials        540      536       -4
Impacted Files Coverage Δ Complexity Δ
.../watson/developer_cloud/service/WatsonService.java 56.28% <22.22%> (-2.58%) 42 <7> (+1)
...eloper_cloud/service/security/IamTokenManager.java 63.79% <50%> (-1.67%) 12 <0> (+1)
...m/watson/developer_cloud/util/CredentialUtils.java 74.71% <85.29%> (+13.84%) 26 <9> (-4) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9681f5d...0a30e16. Read the comment docs.

@lpatino10
Copy link
Contributor Author

@mediumTaj could you take another look real quick? I just had to add some null checks.

Copy link
Contributor

@mediumTaj mediumTaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 looks good!

@lpatino10 lpatino10 merged commit 0b12911 into master Jan 9, 2019
@lpatino10 lpatino10 deleted the handle-bad-credential-chars branch January 9, 2019 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants